Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sets up structs (pseudo-structs) as a replacement for datums for si… #7077

Closed
wants to merge 5 commits into from

Conversation

boskoramen
Copy link
Contributor

@boskoramen boskoramen commented Sep 1, 2024

About the pull request

  • Instead of creating and using datums for storing data, can just initialize simple lists
  • Syntax is pretty clunky/verbose, open to suggestions for how to improve it for readability and elegance
  • Controller logic mostly pulled from the global vars controller

Explain why it's good for the game

Pros:

  • Do not need to create and delete datums
  • Straightforward interface

Cons:

  • Verbose syntax
  • Debugging can be annoying since there is a lot of C macro stuff under the hood

This was a fun project to do over the weekend, wanted to see if this was doable after found a mini use case for unit tests I was planning to write. Can't really think of anything that can directly use this except for launch metadata at the moment, curious to see if people can think of other stuff.

Testing Photographs and Procedure

Screenshots & Videos

Tested locally with warrior throw, human throw, and explosions. Explosion lag caused some issues with indexing the list but looking at variables in debugger not sure if this is an issue with the structs or with throw logic and lag.

Changelog

🆑 TheDonkified
code: Added pseudo structs construct for storage of simple data
/:cl:

@github-actions github-actions bot added the Code Improvement Make the code longer label Sep 1, 2024
@boskoramen boskoramen changed the title Setups up structs (pseudo-structs) as a replacement for datums for si… Sets up structs (pseudo-structs) as a replacement for datums for si… Sep 1, 2024
…le data storage

- Replaces launch metadata datum with a struct

- Some minor variable renaming, not super comprehensive tho
Copy link
Contributor

@Zonespace27 Zonespace27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting concept, but I have some concerns.

Do not need to create and delete datums

This isn't especially an issue. The cost of creating or deleting a datum is insignificant, and in cases like launch_metadata, the datum would be automatically GCed once it goes out of scope.

Straightforward interface

Somewhat. However, there lie two issues in this. First, this is a code feature that would be unique to CM, making it more complicated for coders from other codebases to understand. Secondly, it seems you lose the ability to cast member variables in a struct, as opposed to the more traditional datum.

Debugging can be annoying since there is a lot of C macro stuff under the hood

This is absolutely a massive concern. This level of macro abuse makes it much harder to debug things.


Overall, the concept is interesting but I don't see what it brings to the table over regular datums. I'd like to hear more justification for this PR.

- Refactored way to get props from structs and delete structs to be less verbose
- Fixed runtime bug when getting thrown mid throw
@boskoramen
Copy link
Contributor Author

Think main benefit is memory usage, though doing some profiling don't think our datum count will scale enough for the memory benefit to matter.

Gonna close this PR, was a fun exercise but my original understanding of datum init/delete cost was misguided.

@boskoramen boskoramen closed this Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Improvement Make the code longer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants